Ensure correct SPN when calling SspiContextProvider #3347

twsouthwick · 2025-05-14T20:23:23Z

This change does the following:

Only initialize when handshake is complete
The serverSpn field is now local to the method
Adds some bookkeeping to ensure we're only using the SPN that has previously generated a context once one has been created.
Reworks the SNI pathways on .NET Core to only return the SPN that is needed. On .NET Core, it was returning an array but only using a single value from that. This refactors that to be more readable to just return the value used.

Part of #2253

This change also adds some book keeping to ensure we're only using the spn that has previously generated a context once one has been created.

codecov · 2025-05-16T03:03:44Z

Codecov Report

Attention: Patch coverage is 75.47170% with 13 lines in your changes missing coverage. Please review.

Project coverage is 58.88%. Comparing base (8eb9f32) to head (acc7602).
Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
...crosoft/Data/SqlClient/SSPI/SspiContextProvider.cs	73.68%	5 Missing ⚠️
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs	50.00%	3 Missing ⚠️
...ata/SqlClient/SSPI/NegotiateSspiContextProvider.cs	70.00%	3 Missing ⚠️
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs	80.00%	1 Missing ⚠️
...soft/Data/SqlClient/ManagedSni/SniProxy.netcore.cs	80.00%	1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (8eb9f32) and HEAD (acc7602). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (8eb9f32) HEAD (acc7602)

addons 1 0

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3347      +/-   ##
==========================================
- Coverage   68.86%   58.88%   -9.98%     
==========================================
  Files         280      271       -9     
  Lines       62417    61907     -510     
==========================================
- Hits        42982    36457    -6525     
- Misses      19435    25450    +6015

Flag	Coverage Δ
addons	`?`
netcore	`67.37% <78.72%> (-5.38%)`	⬇️
netfx	`63.81% <72.41%> (-4.23%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/NegotiateSspiContextProvider.cs

paulmedynski

Some comments about handling multipe and single SPNs.

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/NegotiateSspiContextProvider.cs

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/SspiContextProvider.cs

paulmedynski

Whoops - hit Comment instead of Request Changes

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/SspiContextProvider.cs

paulmedynski · 2025-07-09T09:19:28Z

/azp run

azure-pipelines · 2025-07-09T09:19:55Z

Azure Pipelines successfully started running 2 pipeline(s).

paulmedynski · 2025-07-09T09:20:11Z

@twsouthwick - Did you have to resolve any conflicts from your previous commits when merging main? It's hard to tell from the large merge commit.

twsouthwick · 2025-07-09T17:29:27Z

@paulmedynski I did have to resolve a couple of things - mostly that I had added a type into the SNI directory, and it has been renamed to ManagedSni. I did the merge resolution on GH and hadn't built locally... I've pushed a fix that should succeed

paulmedynski · 2025-07-14T19:53:26Z

/azp run

azure-pipelines · 2025-07-14T19:53:48Z

Azure Pipelines successfully started running 2 pipeline(s).

paulmedynski · 2025-07-15T10:47:19Z

/azp run

azure-pipelines · 2025-07-15T10:47:42Z

Azure Pipelines successfully started running 2 pipeline(s).

twsouthwick · 2025-07-15T14:57:54Z

@mdaigle fixed a merge error that only failed on a few builds so it's ready now :) thanks for all the feedback!

* Reset negotiateAuth if SNI doesn't work This change also adds some book keeping to ensure we're only using the spn that has previously generated a context once one has been created. * initialization only after success * move serverSpn to be local

* Reset negotiateAuth if SNI doesn't work This change also adds some book keeping to ensure we're only using the spn that has previously generated a context once one has been created. * initialization only after success * move serverSpn to be local Co-authored-by: Taylor Southwick <tasou@microsoft.com>

twsouthwick mentioned this pull request May 14, 2025

Expose SspiAuthenticationParameters on SspiContextProvider #2454

Merged

twsouthwick force-pushed the reset-negotiate branch 2 times, most recently from 5610fa9 to 6ee0ae5 Compare May 15, 2025 18:11

twsouthwick requested a review from a team as a code owner May 15, 2025 18:11

twsouthwick force-pushed the reset-negotiate branch from 6ee0ae5 to 74afef8 Compare May 15, 2025 18:13

Reset negotiateAuth if SNI doesn't work

b375f91

This change also adds some book keeping to ensure we're only using the spn that has previously generated a context once one has been created.

twsouthwick force-pushed the reset-negotiate branch from 74afef8 to b375f91 Compare May 15, 2025 18:28

twsouthwick added 6 commits May 15, 2025 13:06

initialization only after success

fbd62e0

move serverSpn to be local

65a6ddf

update messaging

18cf7bd

reduce unnecessary assignments

b448eb0

make sure to raise error if sspi not created

43dff4d

spelling

7e974cf

vonzshik reviewed May 16, 2025

View reviewed changes

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/NegotiateSspiContextProvider.cs Show resolved Hide resolved

paulmedynski reviewed May 16, 2025

View reviewed changes

paulmedynski requested changes May 16, 2025

View reviewed changes

paulmedynski reviewed May 16, 2025

View reviewed changes

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/SspiContextProvider.cs Outdated Show resolved Hide resolved

twsouthwick mentioned this pull request May 16, 2025

Add hook for custom SSPI context for SqlConnection instances #2253

Open

9 tasks

per feedback

f6feaf9

twsouthwick requested review from vonzshik and paulmedynski May 16, 2025 15:09

paulmedynski reviewed May 16, 2025

View reviewed changes

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/SspiContextProvider.cs Outdated Show resolved Hide resolved

paulmedynski approved these changes May 16, 2025

View reviewed changes

fix logic

a2713c9

paulmedynski previously approved these changes May 16, 2025

View reviewed changes

vonzshik reviewed May 16, 2025

View reviewed changes

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/SspiContextProvider.cs Outdated Show resolved Hide resolved

only use second

99335ca

vonzshik approved these changes May 16, 2025

View reviewed changes

only return the required resolved spn

806b059

twsouthwick dismissed paulmedynski’s stale review via 3ca3de6 May 23, 2025 22:48

paulmedynski previously approved these changes May 26, 2025

View reviewed changes

Merge remote-tracking branch 'origin/main' into reset-negotiate

df48313

twsouthwick dismissed paulmedynski’s stale review via df48313 July 8, 2025 20:31

fix merge breaks

0d4af01

paulmedynski added this to the 6.1.0 milestone Jul 14, 2025

paulmedynski previously approved these changes Jul 14, 2025

View reviewed changes

mdaigle previously approved these changes Jul 14, 2025

View reviewed changes

fix ambiguous name

acc7602

twsouthwick dismissed stale reviews from mdaigle and paulmedynski via acc7602 July 14, 2025 21:13

paulmedynski approved these changes Jul 15, 2025

View reviewed changes

paulmedynski requested a review from mdaigle July 15, 2025 14:50

mdaigle approved these changes Jul 15, 2025

View reviewed changes

mdaigle merged commit 42146a3 into dotnet:main Jul 15, 2025
237 checks passed

mdaigle mentioned this pull request Jul 15, 2025

[6.1] Ensure correct SPN when calling SspiContextProvider (#3347) #3478

Merged

paulmedynski modified the milestones: 6.1.0, 7.0-preview1 Jul 15, 2025

twsouthwick deleted the reset-negotiate branch July 15, 2025 16:36

Ensure correct SPN when calling SspiContextProvider #3347

Ensure correct SPN when calling SspiContextProvider #3347

Conversation

twsouthwick commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

paulmedynski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paulmedynski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paulmedynski commented Jul 9, 2025

Uh oh!

azure-pipelines bot commented Jul 9, 2025

Uh oh!

paulmedynski commented Jul 9, 2025

Uh oh!

twsouthwick commented Jul 9, 2025

Uh oh!

paulmedynski commented Jul 14, 2025

Uh oh!

azure-pipelines bot commented Jul 14, 2025

Uh oh!

paulmedynski commented Jul 15, 2025

Uh oh!

azure-pipelines bot commented Jul 15, 2025

Uh oh!

twsouthwick commented Jul 15, 2025

Uh oh!

Uh oh!

Uh oh!

twsouthwick commented May 14, 2025 •

edited

Loading

codecov bot commented May 16, 2025 •

edited

Loading